Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add footer #170

Merged
merged 9 commits into from
Sep 28, 2021
Merged

add footer #170

merged 9 commits into from
Sep 28, 2021

Conversation

kne42
Copy link
Member

@kne42 kne42 commented Sep 22, 2021

Comment on lines 6 to 8
export interface IconPropsColor extends IconProps {
color?: string;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will conflict with my changes in #168, but I like this approach better so that only icons that support changing the color can use it. Although, I imagine if we get to the point where every icon has this prop, we'll be able to consolidate it with the IconProps interface.

One thing is I would name this IconColorProps to be consistent with how prop interfaces are named and I find it more natural that Icon and Color are next to each other since that's what the interface does

Copy link
Contributor

@codemonkey800 codemonkey800 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM! Left one comment about the interface.

I would say let's merge this before #168 so I can get the IconColorProps interface

Copy link
Contributor

@codemonkey800 codemonkey800 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also update the links pls. We found broken and copy-and-pasted link during the bug bash:

https://airtable.com/shr6MXADGlNNaohN7/tblKHsTqQzLZW70S9/viwhDGIqzDkq0EprV/rec5BrtMb8BEdhugP

Comment on lines 49 to 52
const COMMON_STYLES = clsx(
'text-xs screen-450:text-sm text-white',
'whitespace-nowrap mr-6 last:mr-0',
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry if there wasn't autocomplete for this, i need to add a Taiwind autocomplete pattern for this use case 😭

},
{
title: 'Twitter',
link: 'https://twitter.com/napari-imaging',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be an underscore lol

https://twitter.com/napari_imaging

},
{
title: 'Zulip',
link: 'https://image.sc',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update this with the zulip link pls

codemonkey800 added a commit that referenced this pull request Sep 23, 2021
codemonkey800 added a commit that referenced this pull request Sep 23, 2021
* `color` prop for icon

* Refactor GridTableOfContents to use icon color prop

* `gridTOC` frontmatter data

* GridTableOfContents component

* Use GridTableOfContents in App

* Use $min-font-size in global.scss

* Fix styling for grid TOC

* Rename GridTableOfContents to QuickLinks

* Remove demo data

* Fix quick links grid gap

* `metaDescription` config for setting meta description tag

* Fix _modules link on API references

* Inline code styling copied from napari hub

* Add custom Pygments code theme

* Load scripts and stylesheets from HTML file

* Fix styling for copy button

* Add comments

* Fix theme colors to be a11y friendly

* Fix sphinx script loading for pages transitions

* Use IconColorProps from #170
@kne42
Copy link
Member Author

kne42 commented Sep 24, 2021

note that adding the napari hub logo messes up some of the styling. I've talked with lia and we've decided to address this later
Screen Shot 2021-09-23 at 4 20 48 PM
Screen Shot 2021-09-23 at 4 22 44 PM

@codemonkey800
Copy link
Contributor

note that adding the napari hub logo messes up some of the styling. I've talked with lia and we've decided to address this later
Screen Shot 2021-09-23 at 4 20 48 PM
Screen Shot 2021-09-23 at 4 22 44 PM

Sounds good to me! I'll also reduce the priority of https://airtable.com/shrh6xgDr4WOIWJ6v/tblKHsTqQzLZW70S9/viwF28gahcRIbKSJ9/reclC9lrwm4sHhIRx

@kne42 kne42 merged commit b1e86c5 into napari:main Sep 28, 2021
@kne42 kne42 deleted the footer branch September 28, 2021 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants